Run PR validation on multiple OS #312
Conversation
skarpovdev
force-pushed
the
skarpov/workflow
branch
from
de72417 to
ed40f3b
Compare
skarpovdev
force-pushed
the
skarpov/workflow
branch
from
246a477 to
0666928
Compare
skarpovdev
force-pushed
the
skarpov/workflow
branch
from
f780f76 to
41b65a1
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the CI workflow to run validation on multiple operating systems (macOS and Ubuntu) with different test configurations. The primary motivation is to support Docker-dependent integration tests only on Ubuntu, since macOS machines don't have Docker preinstalled.
- Splits the single validation job into a matrix build with macOS and Ubuntu configurations
- Adds conditional test exclusion for Docker-dependent tests on macOS
- Separates full build validation (macOS) from integration test validation (Ubuntu)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kotlin-sdk-test/build.gradle.kts | Adds conditional exclusion of TypeScript tests when excludeDockerTests system property is true |
| .github/workflows/build.yml | Converts single job to matrix strategy with OS-specific configurations and test exclusions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| useJUnitPlatform() | ||
| if (System.getProperty("excludeDockerTests") == "true") { | ||
| filter { | ||
| excludeTestsMatching("*.typescript.*") |
There was a problem hiding this comment.
The test exclusion pattern *.typescript.* uses a generic wildcard that may inadvertently exclude tests beyond Docker-dependent ones. Consider using a more specific pattern like *DockerTest* or *IntegrationTest* to clearly identify which tests require Docker, making the exclusion logic more maintainable and less prone to accidentally excluding unrelated tests.
| excludeTestsMatching("*.typescript.*") | |
| excludeTestsMatching("*DockerTest*") |
Copilot uses AI. Check for mistakes.
| include: | ||
| - os: macos-latest-xlarge | ||
| name: "macOS" | ||
| java-opts: "-Xmx8g -Dfile.encoding=UTF-8 -Djava.awt.headless=true -Dkotlin.daemon.jvm.options=-Xmx6g -DexcludeDockerTests=true" |
There was a problem hiding this comment.
The excludeDockerTests system property is set here but referenced as excludeDockerTests in the Gradle file, while the Gradle code checks for System.getProperty("excludeDockerTests"). Consider using a more descriptive property name like excludeDockerDependentTests to make the purpose clearer and ensure consistency between the workflow and build script.
| java-opts: "-Xmx8g -Dfile.encoding=UTF-8 -Djava.awt.headless=true -Dkotlin.daemon.jvm.options=-Xmx6g -DexcludeDockerTests=true" | |
| java-opts: "-Xmx8g -Dfile.encoding=UTF-8 -Djava.awt.headless=true -Dkotlin.daemon.jvm.options=-Xmx6g -DexcludeDockerDependentTests=true" |
Copilot uses AI. Check for mistakes.
1 participant
Motivation and Context
The changes are needed for #310 to run integration tests which rely on Docker only on Ubuntu, since macOS machines don't have Docker preinstalled.
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context